Skip to content

Conversation

@widlarizer
Copy link
Collaborator

In #5417, several changes ended up leaking memory at Yosys shutdown. This wasn't caught by CI, since we only run sanitizers on main. While these changes fix it, there's a couple of questions I have:

  • should ID(...) constructed strings really be immortal? They've been possibly incorrectly used in some pmgen FPGA backends
  • is there a better way to GC at shutdown than in yosys_atexit with the same code but without marking anything live?

I observed the leaks with SANITIZER=undefined,address but probably only address is necessary to reproduce it.

Paging @rocallahan (I'd request a review but the reviewer selection window won't let me select non-members)

@widlarizer widlarizer requested a review from mmicko November 13, 2025 13:16
Copy link
Member

@mmicko mmicko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm that sanitizers are not failing anymore https://github.com/YosysHQ/yosys/actions/runs/19332922878

@widlarizer
Copy link
Collaborator Author

If there's alternate solutions let's explore them in a followup, for now let's just make main go green again

@widlarizer widlarizer merged commit 1929956 into main Nov 13, 2025
33 checks passed
@rocallahan
Copy link
Contributor

Thanks for fixing this.

  • should ID(...) constructed strings really be immortal? They've been possibly incorrectly used in some pmgen FPGA backends

Yes, I intended ID() strings to be immortal. There's a limited number of them so that should be OK.

  • is there a better way to GC at shutdown than in yosys_atexit with the same code but without marking anything live?

At shutdown, I would expect there to be no live designs, so tracing shouldn't trace anything, so it's not clear to me why you need to make tracing conditional there.

I did not think about shutdown leaks triggering sanitizer warnings, sorry. I actually designed the GC scheduling with the intent that we not GC on shutdown --- in production, if the process is about to exit, doing cleanup work is a waste of time. But I understand why you'd want to catch bugs with the sanitizers. You might want to consider doing cleanup-before-shutdown work only when sanitizers are enabled.

@mmicko mmicko deleted the emil/idstring-memory-safety branch November 14, 2025 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants